Repository navigation
Conversation
…s natively apache/iceberg-rust#3327 gives `PrimitiveLiteral` an equality and ordering that follow Java's `Float.compare` / `Double.compare`, so iceberg-rust's writers keep -0.0 and 0.0 partitions apart, as iceberg-java does. Move the pin from af1da4c to 1f3bc34, the merge commit of that PR, and drop the fallback that apache#6531 added for float and double partition fields (apache#6138). Two other upstream changes in the range need code changes: - apache/iceberg-rust#3145 makes `FileScanTaskDeleteFile`'s fields private behind a validating builder, and holds the deletion-vector offset and size as u64. The planner builds delete files through the builder and rejects negative coordinates, and the scan sizes a delete file by rebuilding it. - apache/iceberg-rust#3323 fixes iceberg-rust's `day` for pre-epoch timestamps (apache/iceberg-rust#3315). Comet computes those partition values itself, so only the canary that pins iceberg-rust's values changes.
andygrove
added
run-iceberg-tests
run-all-spark-profiles
github-actions
Bot
added
enhancement
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the entire 12-file diff from bfacc43a50aba13da4fa8a828db52309541191b4 to 537488876ef45fe718900e3da9d4a88376456f77. The PR is not a draft. The snapshot and refreshed discussion contained no existing reviews or comments. One introduced P2 issue was identified.
Routed skills: review-comet-pr, review-comet-iceberg-write-pr, and review-comet-expression-pr for the serde eligibility change. Read AGENTS.md and the relevant contributor guidance.
Summary
- Prior state and problem: The base falls back to iceberg-java for float/double partitions because iceberg-rust merged signed zeros, potentially losing rows during partition pruning.
- Design approach: Advance iceberg-rust through its signed-zero fix, remove the fallback, and adapt delete-file construction to the new validated builder API.
- Correctness: The upstream equality now distinguishes signed zeros while treating NaN payloads alike. Both Comet partition-splitting paths use that equality. Native writer probes and Java manifest readers preserved the expected partitions and record counts. No introduced runtime correctness issue meeting the P1/P2 bar was identified.
- Compatibility analysis: Checked Spark
SQLOrderingUtiland write-ordering sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0, plus iceberg-java comparators for 1.5.2, 1.8.1, 1.10.0, and 1.11.0. Spark ties signed zeros when sorting, whereas Iceberg partition keys distinguish them. The tests account for that distinction. The new detection-test tuples do fail the Scala 2.12 strict build, as detailed in the finding. - Key design decisions: Keeping Comet’s timestamp partition kernels is appropriate because the upstream day fix does not resolve every Java/Rust timestamp difference. The remaining eligibility guards and Java metric reconstruction remain intact.
- Implementation sketch: The Scala change removes the floating-point rule and reflection helper. Rust converts deletion-vector coordinates with checked unsigned conversions and rebuilds sized delete files while forwarding every other field. Tests cover builder rejection and field preservation.
- Performance: Delete-file stat requests remain deduplicated, and Puffin files still avoid unnecessary HEAD requests. The builder migration retains comparable per-file copying. No evidence-backed P1/P2 performance regression was identified. No benchmark was run or speedup quantified.
- Design: Fixing value semantics upstream and removing the temporary gate is straightforward. Reviewing the pin as a behavioral change also covered manifest serialization, nested null arrays, decimal page pruning, temporal transforms, and Azure path handling.
- Abstraction & complexity:
sized_delete_filecentralizes the required builder plumbing without introducing another grouping mechanism. Its preservation test makes the migration easier to verify. No additional complexity issue met the reporting bar. - Behavioral changes worth calling out: Float/double identity partitions become native again relative to the base. Compared with
branch-1.1, which admitted these writes using the older equality, the signed-zero separation is an intended correctness improvement. Malformed deletion-vector coordinates now fail during planning. Comet’s timestamp write calculations remain unchanged. - Suggested improvements: Construct the three detection-test entries as explicit nested tuples so Scala 2.12 compiles them under
strict-warnings. No additional P1/P2 improvement was identified.
Exact-head CI: Strict Scala warnings (Spark 3.5, JDK 17) failed on the three new tuple expressions. At the final inspection, the head had 21 successful checks, one failure, seven running checks, and 44 skipped checks. Native builds and the labeled Iceberg/profile runs had not produced complete test verdicts. Both run-iceberg-tests and run-all-spark-profiles are present.
Validation: 161 focused native Iceberg tests and the deletion-vector planner test passed. A disposable real-writer probe exercised both writer modes, top-level and nested float/double fields, signed zeros, different NaN payloads, nulls, infinities, and batch boundaries. All four pinned iceberg-java readers decoded its four manifests with the expected partition bit patterns and record counts. The Scala compiler reproduction failed for the added syntax and passed with explicit nested tuples. Temporary test changes were removed and the checkout is clean.
Limits: Full JVM suites, upstream Spark/Iceberg SQL suites, and object-store integration were not run locally. Pending CI remains a validation limitation. Local native compilation required temporary JNI headers for the installed Java runtime.
| "double", | ||
| "-0.0") | ||
| Seq( | ||
| "part_float" -> ("v FLOAT", "v", "CAST(1.5 AS FLOAT)"), |
There was a problem hiding this comment.
[P2] Construct these entries as explicit nested tuples. The three new "part_*" -> (...) expressions trigger Scala 2.12’s adapted-argument warning because -> accepts one argument and the compiler implicitly creates a three-tuple. The supported Spark 3.5 strict-warnings build promotes this warning to an error, so test-compile fails before tests can run. Using ("part_float", ("v FLOAT", "v", "CAST(1.5 AS FLOAT)")), and the equivalent form for the other entries, avoids the adaptation and restores compilation.
Evidence: Exact-head CI job https://github.com/apache/datafusion-comet/actions/runs/38055061563/job/114222370917 fails at lines 1509–1511 with Adapting argument list by creating a 3-tuple while running ./mvnw -B test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests. Independently compiling the three entries with Scala 2.12.18 and -Xlint:adapted-args -Xfatal-warnings exited 1. The explicit nested-tuple version exited 0.
The lifted gate also admits NaN partition values, so the signed-zero test writes NaN and -NaN through both writers too. The bump changes how iceberg-rust writes a named Avro type that two partition fields share, so a new test writes two decimal identity partitions of the same type, in both encodings, and compares them with iceberg-java. The detection test now uses plain tuples. Scala 2.12 flagged the adapted argument list as an error under -Pstrict-warnings.
- One closure converts a delete file's record count and blob coordinates from the proto's int64, instead of three copies of the same block. - The planner test keeps the cases Comet's own conversion decides, and drops the two that only exercised iceberg-rust's validation messages. - `committedPartitions` sits after `partitionDirs`, replaces the far-date test's local copy of the same query, and is read once per assertion. - The detection test uses the suite's `createTable` table, so `assertSupportLevelIs` keeps its original signature.
2 participants
Which issue does this PR close?
No issue. This lifts one of the restrictions listed in #5643: the fallback that #6531 added for #6138.
Rationale for this change
Since #6531, a native Iceberg write falls back to iceberg-java when the output partition spec has a
floatordoublefield. iceberg-rust compared float partition values withOrderedFloat's equality, which treats-0.0and0.0as one value. Both of the native writer's grouping paths then filed rows with either value under whichever arrived first, and a read that pruned on the other value lost rows (#6138).apache/iceberg-rust#3327 gives
PrimitiveLiteralhand-writtenPartialEqandPartialOrdthat follow Java'sFloat.compareandDouble.compare:-0.0sorts before0.0, and all NaNs are equal. That is how iceberg-java compares partition keys, so with the pin past it the fallback can go.What changes are included in this PR?
icebergandiceberg-storage-opendalpin moves fromaf1da4cto1f3bc34, the merge commit of fix(spec): treat -0.0 and 0.0 as distinct float literals iceberg-rust#3327. That is 17 commits.Cargo.lockpicks up apache-avro 0.22 and nine new crate versions (ouroboros, strum 0.28, aliasable and their helpers).CometIcebergNativeWritedrops the float and double partition rule, andIcebergReflectiondrops its helper.FileScanTaskDeleteFile's fields private behind a builder that validates onbuild(), and holds the deletion-vector offset and size asu64. The planner builds delete files through the builder and rejects a negative record count, offset or size from the proto.IcebergScanExec::fill_delete_file_sizessizes a delete file by rebuilding it, as it already does for the task. The JVM serde already sends every field the builder requires for a deletion vector.dayfor pre-epoch timestamps (daytransform puts some pre-epoch timestamps in the next day iceberg-rust#3315). Comet computes the time-transform partition values of timestamps itself, so nothing it writes changes. The canary test that pins iceberg-rust's values now expects the fixedday. iceberg-rust still differs from iceberg-java for a timestamp exactly 999999 microseconds into a unit (Native Iceberg years/months/days/hours differ from Iceberg for some pre-1970 timestamps #6426), so Comet keeps computing these values.The rest of the range needs no Comet changes:
decimal_10_2, once for each field. Now it defines the type once and refers to it after that. A new test checks that iceberg-java still reads the native manifests.FIXED_LEN_BYTE_ARRAYdecimals (feat(reader): page-index pruning for FIXED_LEN_BYTE_ARRAY decimals iceberg-rust#3328). Comet doesn't push residual predicates on decimal columns to iceberg-rust (IcebergReflection.pageIndexUnsupportedColumns), so this doesn't reach the native scan yet.CometScanRulealready falls back for V3 columns with defaults.The nested schema evolution guard from #6504 stays, because it waits on apache/iceberg-rust#3255, which is still open.
How are these changes tested?
CometIcebergWriteActionSuite"signed-zero and NaN float and double identity partitions match iceberg-java" requires the native writer to run.-0.0with0.0, and a clustered write that returns to an earlier zero fails on iceberg-java too.libcometbuilt frommainwith the fallback removed, both writers put every zero underf=-0.0/d=0.0, so the test fails. With this pin it passes.decimal(10, 2)anddecimal(38, 2)pairs natively and compares them with iceberg-java.CometIcebergWriteDetectionSuitereplaces four fallback tests with one that checks a double identity partition isCompatible.datafusion-cometlib tests;maintoo);A pin bump can change file bytes and manifests, and the other Spark profiles pin Iceberg 1.5.2, 1.8.1 and 1.10.0. So this PR is labelled
run-iceberg-testsandrun-all-spark-profiles.